tests/streams/order_book/fee_distribution_test.go (1)

1170-1171: Consider using a fee amount that produces a non-zero remainder.

With totalFees = 30 TRUF, the infraShare = 3.75 TRUF divides evenly by 3 validators (1.25 TRUF each), resulting in zero remainder. The remainder-handling logic (lines 1237-1250) won't be exercised.

Consider using a fee like 31 TRUF which gives infraShare = 3.875 TRUF, producing a non-trivial remainder when divided by 3.

💡 Suggested change to test remainder handling

-		totalFees := new(big.Int).Mul(big.NewInt(30), big.NewInt(1e18))
+		// Use 31 TRUF to ensure non-zero remainder when splitting among 3 validators
+		// infraShare = 31 * 0.125 = 3.875 TRUF; 3.875 / 3 = 1.291... with remainder
+		totalFees := new(big.Int).Mul(big.NewInt(31), big.NewInt(1e18))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/streams/order_book/fee_distribution_test.go` around lines 1170 - 1171,
The test sets totalFees to 30 TRUF which yields an infraShare that divides
evenly across validators and never exercises the remainder branch; update the
totalFees initializer (variable totalFees) to use a value like 31 TRUF (e.g.,
big.NewInt(31) * 1e18) so infraShare (infraShare) produces a non-zero remainder
and the remainder-handling logic is executed during the test.

internal/migrations/033-order-book-settlement.sql (1)

118-128: Double iteration over validators for counting and distribution.

The code iterates over tn_utils.get_validators() twice: once to count validators (lines 119-121) and again to distribute (line 128). While this works correctly because the result is deterministic within a transaction, it's slightly inefficient.

This is likely a Kuneiform limitation (no way to store the result set or count during iteration). The current approach is acceptable given the constraints.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/migrations/033-order-book-settlement.sql` around lines 118 - 128,
The migration currently iterates tn_utils.get_validators() twice—once to compute
$validator_count and again to distribute $infra_share—which is inefficient;
update the logic in the block around $validator_count, $per_validator,
$val_remainder and the subsequent for loop so you only enumerate validators
once: either (a) if Kuneiform supports materializing the iterator, collect
tn_utils.get_validators() into a single in-memory list/array and derive count
and distribution from that list before distributing, or (b) add/use a helper
like tn_utils.get_validator_count() (or an equivalent COUNT query) to compute
$validator_count without a first iteration, then run the single distribution
loop over tn_utils.get_validators(); adjust references to $validator_count,
$per_validator, $val_remainder and $first_validator accordingly.

tests/streams/order_book/fee_distribution_test.go (2)

1115-1118: Consider adding a test assertion for remainder recipient determinism.

The test description mentions "remainder going to the first (sorted by pubkey)" but the assertions only verify diff <= 1 without confirming which validator receives the remainder. If the specification requires deterministic remainder assignment (sorted by pubkey), consider:

1. Sorting the injected validators by pubkey
2. Asserting the first validator (sorted) receives more than the others

This would validate the deterministic ordering behavior described in the function comment.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/streams/order_book/fee_distribution_test.go` around lines 1115 - 1118,
The test testDistributionMultipleValidators currently only checks fee
differences <= 1; update it to also assert deterministic remainder recipient by
sorting the injected validators by their public key (use the same pubkey field
used in setup) and then assert that the first validator in that sorted order
receives the extra remainder (i.e., its received fee > each other validator's
fee, or that it equals max and at least one other is less), thereby verifying
the comment's "remainder going to the first (sorted by pubkey)" behavior.

---

1231-1250: Remainder handling assertions may be brittle depending on implementation.

The test asserts both diff <= 1 (line 1245) and diff > 0 (line 1249). With 31 TRUF and 3 validators:

• infraShare = 3,875,000,000,000,000,000 wei
• Per-validator = 1,291,666,666,666,666,666.67...

The actual diff depends on how the SQL implementation handles division:

• Truncation + single-recipient remainder: remainder = 2 wei → diff = 2 (fails <= 1 assertion)
• Rounding + negative correction: first validator gets 1 less → diff = 1 (passes)
• Truncation + spread remainder 1 wei each: two validators get +1 each → diff = 1 (passes)

Consider adding a comment documenting the expected implementation behavior, or relaxing the <= 1 constraint to <= 2 if the implementation gives all remainder to one validator.

📋 Suggestion: Add implementation documentation or relax constraint

 		// Payouts should differ by at most 1 wei (remainder handling)
 		// PostgreSQL NUMERIC(78,0) division rounds, so the first validator
 		// gets per_validator + remainder (which may be negative, i.e., 1 wei less)
+		// NOTE: With infraShare=3875000000000000000 / 3, PostgreSQL banker's rounding
+		// gives per_validator=1291666666666666667, total=3875000000000000001,
+		// so remainder=-1 is applied to the first validator.
 		vals := []*big.Int{inc1, inc2, inc3}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/streams/order_book/fee_distribution_test.go` around lines 1231 - 1250,
The two assertions around validator payout differences (variables vals, minVal,
maxVal, diff and the require.True checks) are brittle given varying SQL
division/remainder strategies; update the test to allow a maximum difference of
2 wei (change the first require.True from diff ≤ 1 to diff ≤ 2) and adjust the
second assertion to assert diff ≥ 1 only when the test is meant to guarantee a
non-zero remainder, or remove the strict diff.Sign() > 0 check and instead add a
clarifying comment describing that SQL implementations may assign remainder up
to 2 wei to a single validator; ensure the change updates the message text to
reflect the new bound.

tests/streams/order_book/fee_distribution_audit_test.go (1)

519-534: Defensive code path that may be unreachable with current test setup.

The fallback logic (Lines 524-532) handles the case where User1 is also the validator/leader. However, since injectTestValidator always creates a fresh keypair (see context snippet 1), User1 will never be the validator in this test. This code path may never execute.

Consider either:

1. Removing the fallback if it's dead code
2. Adding a test case that explicitly tests User1 as the validator

This is a minor observation and doesn't affect correctness.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/streams/order_book/fee_distribution_audit_test.go` around lines 519 -
534, The test contains a defensive fallback that adds the validator share to
expectedIncrease1 when increase1 mismatches auditReward1, but given
injectTestValidator always generates a new keypair User1 can never be the
validator so that branch is effectively dead; either remove the unreachable
fallback block (the if that parses totalValFeesStr and updates expectedIncrease1
using infraShareVal) from the test around variables increase1, expectedIncrease1
and auditReward1, or add an explicit test variant that makes User1 the validator
by reusing User1's keypair with injectTestValidator (so the fallback is
exercised) and assert the validator-share-adjusted expectation instead. Ensure
you update or add assertions referencing increase1.String() and
expectedIncrease1.String() accordingly.

internal/migrations/033-order-book-settlement.sql (1)

141-144: Integer division truncates $per_validator; verify remainder handling is sufficient.

The calculation $infra_share / $validator_count performs integer division, which truncates. The remainder is computed and given to the first validator. However, $val_pct is computed as 12.50 / $validator_count which also truncates, and this percentage is added per-validator without accounting for the percentage remainder. Over many validators, the summed percentages in ob_fee_distribution_details may not equal exactly 12.50%.

This is likely acceptable for audit purposes (small rounding error), but worth noting if exact percentage tracking is required.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/migrations/033-order-book-settlement.sql` around lines 141 - 144,
The current math truncates when computing per-validator shares and the
per-validator percentage ($per_validator, $val_remainder, $val_pct) causing lost
fractional percentage mass; change $val_pct to compute using a higher-precision
NUMERIC (e.g., NUMERIC(10,4) or more) so division preserves fractional cents,
then compute a percentage remainder like $pct_remainder := 12.50::NUMERIC(...) -
($val_pct * $validator_count::NUMERIC(...)) and distribute that remainder
deterministically across validators (e.g., add the smallest precision unit to
the first N validators using the existing $first_validator / loop logic) so the
sum written to ob_fee_distribution_details equals exactly 12.50 while keeping
$per_validator/$val_remainder handling consistent.

internal/migrations/033-order-book-settlement.sql (1)

95-99: ⚠️ Potential issue | 🔴 Critical

Create the audit tables before inserting the placeholder row.

ob_fee_distributions and ob_fee_distribution_details are still defined in internal/migrations/036-order-book-audit.sql, so this insert will run before the tables exist and fail during migration replay. Move those table definitions earlier or renumber this migration.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/migrations/033-order-book-settlement.sql` around lines 95 - 99, The
INSERT into ob_fee_distributions is running before the audit tables are created
(they're defined in 036-order-book-audit.sql), causing migrations to fail; fix
by ensuring ob_fee_distributions and ob_fee_distribution_details are created
before you insert the placeholder row: either move the CREATE TABLE statements
from 036-order-book-audit.sql into an earlier migration (before the INSERT in
033-order-book-settlement.sql) or renumber the migration files so that 036
becomes earlier than 033; update references to the INSERT (variables like
$distribution_id, $query_id, $block_count, `@block_timestamp`) remain unchanged.

tests/streams/order_book/fee_distribution_audit_test.go (1)

594-624: Make the proposer explicit instead of silently falling back.

NewTestProposerPub(t) is not registered as a validator in this helper, so a call site that forgets to pass valPub still reaches distribute_fees, but no longer exercises the validator-aware path these tests are meant to cover. Requiring a registered proposer here would fail fast instead of silently downgrading the scenario.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/streams/order_book/fee_distribution_audit_test.go` around lines 594 -
624, The helper fundVaultAndDistributeFees currently silently generates a
proposer via NewTestProposerPub when none is passed, which masks tests that
should exercise the validator-aware path; change fundVaultAndDistributeFees to
require an explicit proposer (remove the fallback that calls NewTestProposerPub)
and return an error immediately if proposer is not supplied, and ensure callers
are updated to pass the intended validator public key; additionally before
calling distribute_fees verify the provided proposer is a registered validator
(or fail fast) so tests can't accidentally skip the validator-aware branch.